-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
LITE-27895 Add create ppr functionality #41
LITE-27895 Add create ppr functionality #41
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me it's ok to merge this code now to be able to rebase on it and to demonstrate the feature.
But I think it might be better to refactor it later. Below I will write my minds about what can be improved.
@@ -12,20 +12,22 @@ class Configuration(Model): | |||
|
|||
PREFIX = 'CFL' | |||
|
|||
STATE = ConfigurationStateChoices |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a lot of refactoring in several files. I think it is more convenient to make a separate commit for refactoring.
@@ -68,7 +68,11 @@ class ConfigurationCreateSchema(NonNullSchema): | |||
file: FileSchema | |||
|
|||
|
|||
class ConfigurationSchema(NonNullSchema): | |||
class PPRCreateSchema(NonNullSchema): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description could be provided by user so it is required here. But I have it in my PR, so I will add it after merge.
return new_version | ||
|
||
|
||
def create_ppr(ppr, context, deployment, db, client, logger): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is very big and has a lot of logic. It might be more convenient to refactor it and to split to several separate functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree here, at least there are two branches that can be done as separate functions --> uploading and generation
summary.update({'errors': errors}) | ||
status = PPRVersion.STATUS.failed | ||
try: | ||
file_instance = File( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose to leave this logic related to models in webapp.py for uniformity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree on reduce the create function because is really big, but all related to file management, creating object is more related to the service layer than the webapp.
connect_ext_ppr/service.py
Outdated
.filter_by( | ||
deployment=deployment.id, | ||
state=Configuration.STATE.active, | ||
).first() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to use .one_or_none(), I suppose, to catch situation if there are more then one active configuration.
) | ||
items = list(get_product_items(client, deployment.product.id)) | ||
|
||
file, writer, wb = get_base_workbook(data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All this file operations might be moved to the separate function
@@ -11,7 +11,7 @@ | |||
<link rel="preconnect" href="https://fonts.gstatic.com" crossorigin> | |||
<link href="https://fonts.googleapis.com/css2?family=Roboto:ital,wght@0,400;0,500;0,700;1,400;1,500;1,700&display=swap" rel="stylesheet"> | |||
<title>Index</title> | |||
<script defer src="vendors.c37f1993399243c8d2a4.js"></script><script defer src="index.f0239835f73a360467d7.js"></script><link href="index.5c5477c3e81eb95e1456.css" rel="stylesheet"></head> | |||
<script defer src="vendors.0b82e61f94dfa88a3bd2.js"></script><script defer src="index.acad834064fd73a90d98.js"></script><link href="index.5c5477c3e81eb95e1456.css" rel="stylesheet"></head> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do these changes occur? I have the same in my PRs. Maybe there is a way to prevent this?
connect_ext_ppr/service.py
Outdated
f"(product_id={deployment.product_id}, " | ||
f"product_version={deployment.product.version})" | ||
) | ||
print(previous_ppr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
print?
return new_version | ||
|
||
|
||
def create_ppr(ppr, context, deployment, db, client, logger): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree here, at least there are two branches that can be done as separate functions --> uploading and generation
265df88
to
9691966
Compare
9691966
to
d64ac54
Compare
Changes on this PR: